Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a compile time error when passing arguments to regular HTML elements (e.g. <a @foo=) #1102

Conversation

fivetanley
Copy link
Contributor

@fivetanley fivetanley commented May 18, 2020

Adds an error message in the compile phase to inform the developer that
@arguments are not allowed on HTML nodes. This can often come from
copy/pasting refactoring errors.

For example, given the following template:

<a href="https://emberjs.com" @onClick={{action "foo"}}>Ember.js</a>

Before, the developer would get a vague error at runtime (Example taken from
ember.js#18951
):

jquery.js:3827 Uncaught TypeError: func is not a function
    at StatementCompilers.compile (VM32 ember.debug.js:43121)
    at compileStatements (VM32 ember.debug.js:44323)
    at maybeCompile (VM32 ember.debug.js:44312)
    at CompilableTemplateImpl.compile (VM32 ember.debug.js:44292)
    at Object.evaluate (VM32 ember.debug.js:51011)
    at AppendOpcodes.evaluate (VM32 ember.debug.js:49382)
    at LowLevelVM.evaluateSyscall (VM32 ember.debug.js:52284)
    at LowLevelVM.evaluateInner (VM32 ember.debug.js:52240)
    at LowLevelVM.evaluateOuter (VM32 ember.debug.js:52232)
    at JitVM.next (VM32 ember.debug.js:53175)

Now, they get an error message that looks like:

SyntaxError: ${attribute.name} is not a valid attribute name. @arguments
are only allowed on components, but the tag for this element
(\`${elementNode.tag}\`) is a regular, non-component HTML element. (location
line and column)

Given that there's no special behavior glimmer adds for @arguments on regular
HTML nodes, and it seems invalid to put @ in attribute names in regular HTML
(validator.w3c.org gives Attribute @onclick is not serializable as XML 1.0
error message for the example template above), it seems helpful for the glimmer
compiler to fail at build time to prevent copy/paste errors and mistakes.

Fixes emberjs/ember.js#18951

@fivetanley fivetanley changed the title disallow @arguments on regular HTML nodes compile time error: disallow @arguments on regular HTML nodes May 18, 2020
@fivetanley fivetanley changed the title compile time error: disallow @arguments on regular HTML nodes [bugfix] compile time error: disallow @arguments on regular HTML nodes May 18, 2020
@fivetanley fivetanley force-pushed the disallow-at-arguments-on-regular-html-nodes branch from 3abccbf to f27f3f7 Compare May 18, 2020 16:19
Adds an error message in the compile phase to inform the developer that
`@arguments` are not allowed on HTML nodes. This can often come from
copy/pasting refactoring errors.

For example, given the following template:

```handlebars
<a href="https://emberjs.com" @OnClick={{action "foo"}}>Ember.js</a>
```

Before, the developer would get a vague error at runtime ([Example taken from
ember.js#18951](emberjs/ember.js#18951)):

```
jquery.js:3827 Uncaught TypeError: func is not a function
    at StatementCompilers.compile (VM32 ember.debug.js:43121)
    at compileStatements (VM32 ember.debug.js:44323)
    at maybeCompile (VM32 ember.debug.js:44312)
    at CompilableTemplateImpl.compile (VM32 ember.debug.js:44292)
    at Object.evaluate (VM32 ember.debug.js:51011)
    at AppendOpcodes.evaluate (VM32 ember.debug.js:49382)
    at LowLevelVM.evaluateSyscall (VM32 ember.debug.js:52284)
    at LowLevelVM.evaluateInner (VM32 ember.debug.js:52240)
    at LowLevelVM.evaluateOuter (VM32 ember.debug.js:52232)
    at JitVM.next (VM32 ember.debug.js:53175)
```

Now, they get an error message that looks like:

```
SyntaxError: ${attribute.name} is not a valid attribute name. @arguments
are only allowed on components, but the tag for this element
(\`${elementNode.tag}\`) is a regular, non-component HTML element. (location
line and column)
```

Given that there's no special behavior glimmer adds for `@arguments` on regular
HTML nodes, and it seems invalid to put `@` in attribute names in regular HTML
(validator.w3c.org gives ` Attribute @OnClick is not serializable as XML 1.0`
error message for the example template below), it seems helpful for the glimmer
compiler to fail at build time to prevent copy/paste errors and mistakes.

Fixes emberjs/ember.js#18951
@fivetanley fivetanley force-pushed the disallow-at-arguments-on-regular-html-nodes branch from f27f3f7 to f265f28 Compare May 18, 2020 16:21
@@ -55,6 +55,19 @@ function test(desc: string, template: string, ...expectedStatements: BuilderStat
const Append = Builder.Append;
const Concat = Builder.Concat;

QUnit.test(
Copy link
Contributor Author

@fivetanley fivetanley May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't match the test style in the rest of the file, which is a wrapper around QUnit.test and looks at wire format output. i thought it might be more clear to just use QUnit.test directly rather than extend the test function in this file to allow a callback/expect errors. Totally open to change this to whatever the maintainers prefer, let me know!

@rwjblue rwjblue requested a review from chancancode May 18, 2020 17:03
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me, thanks for working on it @fivetanley!

Cross linking some of the other convo on this subject: ember-template-lint/ember-template-lint#927. Also somewhat related to ember-template-lint/ember-template-lint#1334 (though not 100%).

@rwjblue rwjblue added the bug label May 18, 2020
@rwjblue rwjblue changed the title [bugfix] compile time error: disallow @arguments on regular HTML nodes Add a compile time error when passing arguments to regular HTML elements (e.g. <a @foo=) May 18, 2020
@rwjblue rwjblue merged commit 4621c12 into glimmerjs:master May 18, 2020
@fivetanley fivetanley deleted the disallow-at-arguments-on-regular-html-nodes branch May 23, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

passing @arguments to regular html elements doesn't give a great error message
3 participants